-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Date and Time fields #931
Date and Time fields #931
Conversation
ignorantly assume everyone reads a 24 hour clock 🤷♀️
@@ -174,7 +174,7 @@ class User < ActiveRecord::Base | |||
end | |||
end | |||
|
|||
it "assigns dates, times, and datetimes a type of `DateTime`" do | |||
it "assigns dates, times, and datetimes a type of `Date`, `Time` and `DateTime`" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line is too long. [89/80]
spec/features/customer_form_spec.rb
Outdated
end | ||
|
||
def select_from_timepicker(time_string) | ||
page.execute_script(<<-JS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing whitespace detected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good!
There's a minor test failure where the date/time isn't displaying quite right, and I've left a few other comments.
Do you think you could add the new fields to the /docs
, too?
@@ -0,0 +1,5 @@ | |||
class AddBirthdateToCustomer < ActiveRecord::Migration[4.2] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you set the migration to 5.1
? We've been doing that for recent migrations.
@@ -0,0 +1,5 @@ | |||
class AddExampleTimeToCustomer < ActiveRecord::Migration[4.2] | |||
def change | |||
add_column :customers, :example_time, :time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about doing birth_date
and birth_time
to be consistent?
(It's the closest I can think which might make sense and avoid us having an example_time
field.)
spec/lib/fields/date_spec.rb
Outdated
@@ -0,0 +1,48 @@ | |||
require "rails_helper" | |||
require "administrate/field/date" | |||
# require "support/field_matchers" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we lose this comment?
6f310f4
to
3ee0a16
Compare
spec/lib/fields/time_spec.rb
Outdated
formats: { | ||
default: "%r", | ||
short: "%H:%M", | ||
time_without_date: "%H:%M:%S" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put a comma after the last item of a multiline hash.
This is looking good! Do you think you'd also be able to contribute some docs, too? We've got a list of included fields which this should be part of in |
@nickcharlton Sure. I will update docs There is still one failing spec with which I'm fighting. It's related to the timezone handling. I plan to fix it this weekend. |
@nickcharlton I've added fields to the docs, but there is still one issue with date picker. Datepicker tries to show time in the local timezone when original value is in UTC. |
Is that because the time being stored doesn't have a time zone, or is it losing the timezone? Or, is it just doing the Rails thing of assuming you'd want it in a local time? I think as a user I'd expect it to display in the same way I'd stored it, but that makes it difficult if we're not storing that data. |
@nickcharlton We are storing it without timezone and display on the form in the UTC. But, datetime_picker shows it in the local timezone, which brings inconsistency. From my perspective, I'd say that we don't need to save timezone and show exactly save value as saved. I wasn't managed to configure datetime_picker to not add timezone to the value. Still trying. |
@nickcharlton I won't work on this in the nearest feature. I don't use Rails for a while now. If anyone would like to take ownership on this or feel free to just close it. |
This adds date and time fields. #741
Continuation of #757